-
Notifications
You must be signed in to change notification settings - Fork 165
Use simple cache key equality check #911
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Nico Burns <nico@nicoburns.com>
|
@jrmoulton Could you give this a go and see if it resolves the issues you're seeing? This should rebase cleanly on top of of Taffy 0.9 if you don't want to upgrade to |
Signed-off-by: Nico Burns <nico@nicoburns.com>
|
This does solve the case that I had shared with you but I still have another case that is broken that is fixed by aggressively clearing the cache on the text node on every frame. Below is a case with your fix applied but not aggressively clearing the cache. If I aggressively clear the cache it doesnt' wrap. bad-wrap.mp4For more context: This case I am aggressively clearing the cache but without your fix applied and it doesn't get enough space. If I use your fix (even without manually clearing the cache) this case works. did-not-wrap.mp4 |
Signed-off-by: Nico Burns <nico@nicoburns.com>
Signed-off-by: Nico Burns <nico@nicoburns.com>
|
@jrmoulton I've pushed another update that you may wish to test. Expect terrible performance with this one. But that ought to be fixable if it solves the correctness issues. |
|
This change does fix all issues I was having without me doing any additional clearing of the cache ❤️ |
f934a0a to
725027a
Compare
|
Hmm... I think this may only be working because it's thrashing the cache (effectively aggressively clearing it for us). When I try to get the performance back it breaks again on the layouts I'm testing. @jrmoulton Your layout looks a particularly simple example that stills breaks, and I would be keen to turn into a test case. Would you be able to post independently runnable code (even Floem code) that reproduces the issue? |
Objective
Ensure that Taffy correctly computes layouts when re-layouting with a warm/populated cache.
Context
Blitz is seeing bugs in the layout that only occur when incremental mode is enabled (which corresponds to having a Taffy cache populated from a previous frame). There are reports of similar bugs from Floem.
In the screenshots below, note how in incremental mode the text in the top-right ("Create Account" and "Log In") wraps, whereas in non-incremental mode it does. It is not supposed to wrap and does not do so in other browsers.
Benchmarks
This appears to have little effect on some benchmarks. But it is a 40-50% regression on the Flexbox "Deep tree (auto size)" benchmarks and the "mixed tree" benchmarks and a 15-25% regression on the CSS Grid "deep tree" benchmarks.
I'm also seeing perf regressions around the 50-80% mark when doing a full (non-incremental) re-layouts of some websites in Blitz. https://en.wikipedia.org/wiki/Barack_Obama is ~36ms -> ~46ms. https://www.bbc.co.uk/news is ~9ms -> 16ms. Layouts with populated cache are very fast ~100 microseconds. I don't have good numbers for the "partial cache" case.
The good news is that it doesn't seem to affect scaling behaviour. It's a ~flat perf regression regardless of tree size.